-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[stable21] Prevent duplicate auth token activity updates #29380
Conversation
The auth token activity logic works as follows * Read auth token * Compare last activity time stamp to current time * Update auth token activity if it's older than x seconds This works fine in isolation but with concurrency that means that occasionally the same token is read simultaneously by two processes and both of these processes will trigger an update of the same row. Affectively the second update doesn't add much value. It might set the time stamp to the exact same time stamp or one a few seconds later. But the last activity is no precise science, we don't need this accuracy. This patch changes the UPDATE query to include the expected value in a comparison with the current data. This results in an affected row when the data in the DB still has an old time stamp, but won't affect a row if the time stamp is (nearly) up to date. This is a micro optimization and will possibly not show any significant performance improvement. Yet in setups with a DB cluster it means that the write node has to send fewer changes to the read nodes due to the lower number of actual changes. Signed-off-by: Christoph Wurst <[email protected]>
04755a6
to
6ac88b4
Compare
Ci says no it seems, restarted |
which job? |
is green, but right before final → moved to 21.0.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking since it would probably result in a regression, as well. See #29678
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs this follow up too: #29681
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should probably get modified to also feature this change
https://github.com/nextcloud/server/pull/29682/files
What is the status? Otherwise closing? |
@ChristophWurst what's the state with it? Close? |
closing |
backport of #29357